Skip to content

branch-4.1: [chore](recycler) Disable enable_mark_delete_rowset_before_recycle and enable_abort_txn_and_job_for_delete_rowset_before_recycle for recycler- #66150 #66150 - #66229

Merged
yiguolei merged 1 commit into
apache:branch-4.1from
wyxxxcat:pick_66150_4_1
Jul 30, 2026

Conversation

@wyxxxcat

Copy link
Copy Markdown
Collaborator

pick: #66150

…and `enable_abort_txn_and_job_for_delete_rowset_before_recycle` for recycler (apache#66150)
@wyxxxcat
wyxxxcat requested a review from yiguolei as a code owner July 29, 2026 08:21
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@wyxxxcat

Copy link
Copy Markdown
Collaborator Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

Cloud UT Coverage Report

Increment line coverage 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 77.58% (1900/2449)
Line Coverage 64.43% (33983/52745)
Region Coverage 64.95% (17524/26980)
Branch Coverage 54.13% (9394/17356)

@wyxxxcat

Copy link
Copy Markdown
Collaborator Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one blocking data-correctness issue in the recycler default change.

Critical checkpoint conclusions:

  • Goal and proof: The diff changes both recycler ownership-safety defaults to false and adapts tests, but every changed lifecycle test forces the previous true/true behavior and does not prove the shipped false/false path safe.
  • Scope and clarity: The source change is small, but it removes a correctness-sensitive claim-before-delete protocol without an equivalent fence or a rationale in the PR body.
  • Concurrency: Blocking issue found. PREPARE-to-tmp promotion can race with queued object deletion, and an eventual transaction left COMMITTED can lose all tmp rows before lazy finalization and still become VISIBLE.
  • Lifecycle/static initialization: The test guard has safe stack lifetime and restores both globals. The relevant production lifecycle is the deferred COMMITTED-to-VISIBLE transition; no separate static-initialization issue was found.
  • Configuration: Both flags are mutable. recycle_tmp_rowsets() observes them in every NORMAL instance mode, while the non-versioned recycle_rowsets() path also loses the fence.
  • Compatibility and parallel paths: Versioned ref-count cleanup, retention delays, rolling instance modes, and compaction/schema-change job checks do not provide equivalent ownership. No wire or storage-format incompatibility was found.
  • Conditional/error handling: Disabling the mark and abort gates is the blocking condition; no separate error-propagation issue was substantiated.
  • Tests/results: The nine guarded lifecycle tests continue to validate true/true only, and existing false/false recycler tests are uncontended. I did not run builds or tests because this review runner explicitly forbids them.
  • Observability: Existing logs can trace these paths, but observability does not prevent the race; no separate logging blocker was found.
  • Persistence and data writes: Formal or tmp metadata can survive while its object files are deleted, and a transaction can become VISIBLE after its tmp metadata and files are gone.
  • Performance: No distinct performance regression was substantiated.

User focus: No additional user-provided review focus was specified.

Review completion: The full two-file authoritative diff and upstream/downstream paths were reviewed. Three rounds of normal and risk-focused review converged; every Round 3 reviewer returned NO_NEW_VALUABLE_FINDINGS against this exact frozen payload, and no suspicious point remains unresolved.

Comment thread cloud/src/common/config.h
CONF_mBool(enable_recycle_delete_rowset_key_check, "true");
CONF_mBool(enable_mark_delete_rowset_before_recycle, "true");
CONF_mBool(enable_abort_txn_and_job_for_delete_rowset_before_recycle, "true");
CONF_mBool(enable_mark_delete_rowset_before_recycle, "false");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Preserve an ownership fence before disabling these defaults

With both defaults false, the non-versioned recycler can select an expired PREPARE rowset and queue object deletion while its recycle key is still present and unmarked. A concurrent commit_rowset() can consume that key and publish tmp metadata, after which the queued worker deletes the now-committed files. The tmp path has the same problem for deferred commits: it can delete/remove all unmarked tmp rows of a transaction left COMMITTED, and TxnLazyCommitTask accepts the later empty scan and advances that transaction to VISIBLE. Versioned ref-counts and retention only narrow the window; they do not establish ownership. Every changed test forces true/true, so none exercises the shipped false/false path. Please keep both fences enabled or add an equivalent atomic claim-before-delete protocol with deterministic race coverage.

@yiguolei
yiguolei merged commit a6589bd into apache:branch-4.1 Jul 30, 2026
30 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants